Skip to content

OSASINFRA-3155 - OpenStack: Create ControlPlaneMachineSet CRDs#7280

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
shiftstack:openstack_cpms
Jul 8, 2023
Merged

OSASINFRA-3155 - OpenStack: Create ControlPlaneMachineSet CRDs#7280
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
shiftstack:openstack_cpms

Conversation

@pierreprinetti
Copy link
Member

No description provided.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 26, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 26, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@pierreprinetti
Copy link
Member Author

/test verify-codegen openstack-manifests unit e2e-openstack-kuryr

@pierreprinetti
Copy link
Member Author

/test verify-codegen openstack-manifests unit e2e-openstack-kuryr

@pierreprinetti pierreprinetti force-pushed the openstack_cpms branch 6 times, most recently from 752525f to 2466500 Compare June 27, 2023 09:46
@pierreprinetti pierreprinetti marked this pull request as ready for review June 27, 2023 13:04
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 27, 2023
@openshift-ci openshift-ci bot requested review from gryf and jhixson74 June 27, 2023 13:05
@openshift-ci openshift-ci bot requested a review from damdo June 27, 2023 15:27
@pierreprinetti
Copy link
Member Author

/cc emilien mandre JoelSpeed damdo

@openshift-ci openshift-ci bot requested a review from mandre June 27, 2023 15:27
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 27, 2023

@pierreprinetti: GitHub didn't allow me to request PR reviews from the following users: emilien.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc emilien damdo mandre

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot requested a review from JoelSpeed June 27, 2023 15:27
@pierreprinetti
Copy link
Member Author

/cc emilienm

@openshift-ci openshift-ci bot requested a review from EmilienM June 27, 2023 15:28
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 27, 2023

@pierreprinetti: GitHub didn't allow me to request PR reviews from the following users: emilien.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc emilien mandre JoelSpeed damdo

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@pierreprinetti
Copy link
Member Author

/retest-required

@EmilienM
Copy link
Member

really good work IMO
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2023
@pierreprinetti
Copy link
Member Author

/test e2e-openstack-kuryr

1 similar comment
@pierreprinetti
Copy link
Member Author

/test e2e-openstack-kuryr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return nil, nil, fmt.Errorf("failed to generate the Machine providerSpec for replica %d: %w", idx, err)
return nil, nil, errors.Wrapf(err, "failed to generate the Machine providerSpec for replica %d", idx)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dave Cheney's pkg/errors has been archived and all its functionality has been incorporated into the standard library. I know the dependency is still available in the Installer's go.mod, however I prefer not to use it in new code.

@r4f4
Copy link
Contributor

r4f4 commented Jul 6, 2023

/approve

/hold
In case you decide to address the nit comments before merging.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 6, 2023
Co-Authored-By: Pierre Prinetti <pierreprinetti@redhat.com>
Co-Authored-By: Emilien Macchi <emilien@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 6, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 6, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: r4f4

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 6, 2023
@EmilienM
Copy link
Member

EmilienM commented Jul 6, 2023

/hold cancel
/lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 6, 2023
}

// No failure domain is exactly like one failure domain with the default values.
if numberOfFailureDomains < 1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if numberOfFailureDomains < 1 {
if numberOfFailureDomains == 0 {

func DefaultRootVolumeAZ() []string {
return []string{""}
// DefaultRootVolumeAZ returns the default value for Root Volume availability zone.
func DefaultRootVolumeAZ() string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Does this need to be a function or can it be a constant ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be a const! Would it be better as a const?

}

// DefaultComputeAZ returns the default value for Compute availability zone.
func DefaultComputeAZ() string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD ccd6238 and 2 for PR HEAD 482a2fe in total

@EmilienM
Copy link
Member

EmilienM commented Jul 6, 2023

/test e2e-aws-ovn e2e-openstack-ovn

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 30838d9 and 1 for PR HEAD 482a2fe in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD a58142c and 0 for PR HEAD 482a2fe in total

@pierreprinetti
Copy link
Member Author

/retest-required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 7, 2023

@pierreprinetti: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-agent-ha-dualstack 482a2fe link false /test e2e-agent-ha-dualstack
ci/prow/e2e-openstack-sdn-parallel 482a2fe link false /test e2e-openstack-sdn-parallel
ci/prow/okd-e2e-aws-ovn 482a2fe link false /test okd-e2e-aws-ovn
ci/prow/okd-scos-e2e-aws-ovn 482a2fe link false /test okd-scos-e2e-aws-ovn
ci/prow/okd-e2e-aws-ovn-upgrade 482a2fe link false /test okd-e2e-aws-ovn-upgrade

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-ci-robot
Copy link
Contributor

/hold

Revision 482a2fe was retested 3 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2023
@pierreprinetti
Copy link
Member Author

/hold cancel
/retry-required

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2023
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD a58142c and 2 for PR HEAD 482a2fe in total

@openshift-merge-robot openshift-merge-robot merged commit 4cb9b2d into openshift:master Jul 8, 2023
@pierreprinetti pierreprinetti deleted the openstack_cpms branch July 8, 2023 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants